Fix OTP type #1283

Sneezry · 2024-08-26T18:17:30Z

We have both OTPType and string definitions for OTP entry types. Previously, OTPType was a numeric enum. When importing OTP URLs, OTP entries were generated with a numeric type value, causing an issue where the period parameter was ignored. This happened because we were comparing OTPType.totp (which has a numeric value of 1) with the string "totp", leading to a false condition. As a result, the system incorrectly identified the entry as not time-based, causing the period parameter to be disregarded. This fix ensures all OTP types are now aligned with the new OTPType string enum, resolving the issue.

mymindstorm · 2024-09-03T02:39:43Z

I feel like there are other places where this change needs to be made. E.g.

Authenticator/src/definitions/module-interface.d.ts

Line 65 in c8a651e

OTPType: number;

We really need to spend some time writing E2E tests. This change is scary to me because this enum is used in a lot of places where type checking is half broken due to vuex.

Sneezry · 2024-09-11T12:41:42Z

Also fixes: #1302 #1292 #1291

@mymindstorm we should have a hotfix ASAP.

mymindstorm · 2024-09-15T00:25:09Z

found a bug:

add a HOTP code
set an encryption password

the hotp code becomes a totp code

mymindstorm · 2024-09-15T00:26:35Z

this also occurs if you add a hotp when encryption is on and reload the extension

mymindstorm

This has a bug in it.

mymindstorm · 2024-09-15T00:36:34Z

src/models/otp.ts

+      this.type = OTPType.hex;
+    } else if ((decryptedData.type as unknown) === 6) {
+      this.type = OTPType.hhex;
+    } else {


This line is causing the bug. This if statement is not considering if decryptedData.type is a valid OTPType already and will always return OTPType.totp when decryptedData.type is a string.

Recommend making a type guard for OTPType and using it whenever we need to check if a type is valid, such as in storage.ts and elsewhere. Plus another shared function to convert from legacy otptype to new version. That would help prevent subtle mistakes like this and help remove all this casting to unknown.

mymindstorm · 2024-09-15T00:43:47Z

src/models/storage.ts

@@ -617,29 +614,53 @@ export class EntryStorage {
      }

      if (!entryData.type) {
-        entryData.type = OTPType[OTPType.totp];
+        entryData.type = OTPType.totp;


move this into the case statement or otherwise refactor so we don't have to cast to unknown below. we can set type: unknown in otp.d.ts/RawOTPStorage

olfek · 2024-09-17T10:17:18Z

src/models/storage.ts

+            type = OTPType.hhex;
+          } else {
+            type = OTPType.totp;
+          }


I recommend keeping numeric enums where you get the benefit of reverse mappings so you don't have to do this ugly thing.

olfek · 2024-09-17T10:23:37Z

src/models/otp.ts

+      this.type = OTPType.hhex;
+    } else {
+      this.type = OTPType.totp;
+    }



(2) I recommend keeping numeric enums where you get the benefit of reverse mappings so you don't have to do this ugly thing.

olfek · 2024-09-27T16:56:42Z

src/models/otp.ts

+    // hex = 5
+    // hhex = 6
+
+    if ((entry.type as unknown) === 1) {


Error OTP type entries would only ever be 1, there is no need to check the other numbers.

See my comment below

olfek · 2024-09-27T16:57:14Z

src/models/otp.ts

+    // hex = 5
+    // hhex = 6
+
+    if ((decryptedData.type as unknown) === 1) {


(2) Error OTP type entries would only ever be 1, there is no need to check the other numbers.

See my comment below

olfek · 2024-09-26T23:11:26Z

Flow in question:

src\background.ts > getTotp()
src\models\storage.ts > EntryStorage > import()
src\models\otp.ts > OTPEntry
src\models\storage.ts > EntryStorage > getOTPStorageFromEntry()
src\models\storage.ts > EntryStorage > get()

So in the currently deployed extension we have this line:

Authenticator/src/models/storage.ts

Line 481 in 6511920

type: (parseInt(data[hash].type) as OTPType) || OTPType[OTPType.totp],

(parseInt(data[hash].type) as OTPType) is always NaN because data[hash].type is a TEXT string.

OTPType[OTPType.totp] gives the string totp but the type of type is the enum OTPType.

From this point onwards, all equality checks on type fail and default type with relevant code is always in use. For example:

Authenticator/src/models/otp.ts

Lines 156 to 160 in 6511920

    
           if (this.type === OTPType.totp && entry.period) { 
        
             this.period = entry.period; 
        
           } else { 
        
             this.period = 30; 
        
           }

Input period is lost and entry is saved with the default value of 30. Other properties may be affected too.

Authenticator/src/models/storage.ts

Lines 221 to 227 in 6511920

    
           const storageItem: RawOTPStorage = { 
        
             encrypted, 
        
             hash: entry.hash, 
        
             index: entry.index, 
        
             type: OTPType[entry.type], 
        
             secret, 
        
           };

Type of type is string, entry.type is a string, OTPType[entry.type] gives a number, a number is stored.

Authenticator/src/models/storage.ts

Lines 624 to 638 in 6511920

    
           switch (entryData.type) { 
        
             case "totp": 
        
             case "hotp": 
        
             case "battle": 
        
             case "steam": 
        
             case "hex": 
        
             case "hhex": 
        
               type = OTPType[entryData.type]; 
        
               break; 
        
             default: 
        
               // we need correct the type here 
        
               // and save it 
        
               type = OTPType.totp; 
        
               entryData.type = OTPType[OTPType.totp]; 
        
           }

Now when loading an entry back in, the number type doesn't match any string type so the switch goes to default.

Pretty significant bug I think, this PR is a high priority one for sure.

olfek · 2024-09-30T10:49:04Z

The cause of this type bug:

Authenticator/src/models/storage.ts

Line 481 in 6511920

type: (parseInt(data[hash].type) as OTPType) || OTPType[OTPType.totp],

https://www.typescriptlang.org/play/?#code/FAehAIGUDsEsAd4FMAu4CMxhOgVwLbgBiA9icAN7Dg3gBCAggErAC+WAxidAM5ooBPZAC5iZcAF5wACngBDAE48kASWgppAImgk0c8HnwAjJAs0BKcHJ5iSlgD73bAbVIkAdIyYBdANxYwKDhEVHAAJiwcAlsIqlp6ZkkMNk5uPnBBZDDRNzCk2UVlNQ1tXSsDAhMzS2sYhydc1zIwz2Y-AIhAXg3AVD2gA

Do you think this is a TypeScript bug? @mymindstorm @Sneezry ?

Sneezry requested a review from mymindstorm August 26, 2024 18:17

Sneezry added 2 commits September 11, 2024 19:02

Fix OTP type

ef89734

convert error otp type

cfd775f

Sneezry force-pushed the zhe/otp-type branch from 675a1d9 to cfd775f Compare September 11, 2024 11:02

fix otp type for export

29b2b95

mymindstorm mentioned this pull request Sep 13, 2024

TOTP code hash algorithm not being detected from QR import #1294

Open

mymindstorm requested changes Sep 15, 2024

View reviewed changes

mymindstorm reviewed Sep 15, 2024

View reviewed changes

olfek reviewed Sep 17, 2024

View reviewed changes

olfek mentioned this pull request Oct 1, 2024

[FIX] OTP type is ignored #1320

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OTP type #1283

Fix OTP type #1283

Sneezry commented Aug 26, 2024

mymindstorm commented Sep 3, 2024

Sneezry commented Sep 11, 2024

mymindstorm commented Sep 15, 2024

mymindstorm commented Sep 15, 2024

mymindstorm left a comment

mymindstorm Sep 15, 2024 •

edited

Loading

mymindstorm Sep 15, 2024 •

edited

Loading

olfek Sep 17, 2024

olfek Sep 17, 2024

This comment was marked as outdated.

This comment was marked as outdated.

olfek Sep 27, 2024

This comment was marked as outdated.

olfek Sep 27, 2024

olfek commented Sep 26, 2024

olfek commented Sep 30, 2024

Fix OTP type #1283

Are you sure you want to change the base?

Fix OTP type #1283

Conversation

Sneezry commented Aug 26, 2024

mymindstorm commented Sep 3, 2024

Sneezry commented Sep 11, 2024

mymindstorm commented Sep 15, 2024

mymindstorm commented Sep 15, 2024

mymindstorm left a comment

Choose a reason for hiding this comment

mymindstorm Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

mymindstorm Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

olfek Sep 17, 2024

Choose a reason for hiding this comment

olfek Sep 17, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

olfek Sep 27, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

olfek Sep 27, 2024

Choose a reason for hiding this comment

olfek commented Sep 26, 2024

olfek commented Sep 30, 2024

mymindstorm Sep 15, 2024 •

edited

Loading

mymindstorm Sep 15, 2024 •

edited

Loading